Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GET Privacy Experience Meta Endpoint #4328

Merged

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Oct 23, 2023

Closes https://github.com/ethyca/fidesplus/issues/1179

Description Of Changes

Add a new GET Privacy Experience Meta endpoint that just returns the meta object for Experiences. Content is really only built for TCF Experiences. If non-TCF Experiences are queried, these objects just have null for the experience meta objects for now. In the future, we could look into building a version hash for experiences with notices too.

Code Changes

Added new endpoint:

curl -X 'GET' \
  'http://localhost:8080/api/v1/privacy-experience-meta?region=us_ca&component=overlay&page=1&size=50' \
  -H 'accept: application/json'

Relevant query params are:

  • region
  • component
  • page
  • size

Steps to Confirm

  • See doc with expected sample requests and responses
  • Experiment with setting TCF Enabled/AC Enabled, neither, and make sure the meta content looks appropriate. This is a public endpoint, no authentication required. It's the same content that is returned in GET Privacy Experience List, just segmented into a simpler endpoint.

Pre-Merge Checklist

…gion, component, and meta info in the response. Only TCF Experiences return any meaningful content at this point.
@cypress
Copy link

cypress bot commented Oct 23, 2023

Passing run #4821 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 8bd42d2 into 30bfb30...
Project: fides Commit: 90ef74cdb0 ℹ️
Status: Passed Duration: 01:05 💡
Started: Oct 25, 2023 8:56 PM Ended: Oct 25, 2023 8:57 PM

Review all test suite changes for PR #4328 ↗︎

…ew AC enabled tag to account for the differences between what is returned in the meta object when AC is enabled versus not enabled
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (30bfb30) 87.83% compared to head (8bd42d2) 87.82%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4328      +/-   ##
==========================================
- Coverage   87.83%   87.82%   -0.01%     
==========================================
  Files         334      334              
  Lines       21079    21113      +34     
  Branches     2742     2748       +6     
==========================================
+ Hits        18515    18543      +28     
- Misses       2099     2101       +2     
- Partials      465      469       +4     
Files Coverage Δ
src/fides/api/schemas/privacy_experience.py 100.00% <100.00%> (ø)
src/fides/common/api/v1/urn_registry.py 100.00% <100.00%> (ø)
...i/api/v1/endpoints/privacy_experience_endpoints.py 91.30% <83.87%> (-2.34%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pattisdr nice, this looks good, and i think you'd be good to merge it as is!

just to raise the question - what would you think of moving this endpoint over to fidesplus? it's not a priority, so if you need/want to get this in as quick as possible, then you can ignore for now. but it seems like a relatively straightforward "wrapping" of underlying functionality that could possibly be moved over without serious rework required?

component: Optional[ComponentType] = None,
request: Request, # required for rate limiting
response: Response, # required for rate limiting
) -> AbstractPage[PrivacyExperienceMetaResponse]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i could be wrong, but isn't the return type annotation here technically AbstractPage[PrivacyExperience]? and then the response_model would be responsible for eliminating in the API response any additional fields on the returned PrivacyExperience that aren't part of the PrivacyExperienceMetaResponse model/schema?

it's a bit academic, and i may be wrong in understanding the functionality/intention! but i was scratching my head for a few seconds trying to reconcile the return type annotation and the objects actually being returned in the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching, it should be PrivacyExperience, this was leftover from when I was still toying with it being just PrivacyExperienceMetaResponse and not wrapped within the PrivacyExperience, but then I thought this should truly be a subset of the PrivacyExperience endpoint contents.

@pattisdr
Copy link
Contributor Author

Discussed - will merge and look into moving later -

@pattisdr pattisdr merged commit 693b22a into main Oct 25, 2023
39 of 40 checks passed
@pattisdr pattisdr deleted the fidesplus_1179_experience_meta_only_PROD-1215_PROD-1236 branch October 25, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants